Honor preinstalled CLI path in .NET MSBuild targets (#921)#1318
Merged
Conversation
The `CopilotSkipCliDownload=true` escape hatch also gated the copy and
register targets, so consumers behind authenticated npm mirrors had no way
to point the build at a pre-downloaded copilot CLI. `_CopilotCliBinaryPath`
was also unconditionally reassigned inside each target, so even setting it
via a global property had no effect.
Add a public `CopilotCliBinaryPath` property that:
* suppresses `_DownloadCopilotCli`
* still runs `_CopyCopilotCliToOutput` and `_RegisterCopilotCliForCopy`
so the supplied binary is placed at the canonical
`runtimes/<rid>/native/copilot[.exe]` path that `Client.GetBundledCliPath`
searches at runtime
* fails the build with an actionable `<Error>` if the path is missing
Switch the `<Copy>` task from `DestinationFolder` to `DestinationFiles`
so off-spec source filenames are normalized to `copilot[.exe]` on copy.
Add `dotnet/test/Unit/MSBuildTargetsTests.cs` -- the first MSBuild-target
test in the repo -- with 5 integration tests that import the real targets
file into a throwaway csproj and shell out to `dotnet build` to validate
the four scenarios (preinstalled, preinstalled + skip, skip only, missing
path) end-to-end.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the .NET SDK’s MSBuild targets to support a first-class “preinstalled Copilot CLI” scenario by honoring a user-supplied CLI binary path, avoiding the unauthenticated DownloadFile path that breaks behind authenticated npm mirrors (issue #921).
Changes:
- Add
CopilotCliBinaryPathto let consumers provide a pre-downloaded CLI and automatically skip_DownloadCopilotCliwhile still copying/registering the binary intoruntimes/<rid>/native/. - Prevent caller-supplied internal properties (e.g.,
_CopilotCliBinaryPath/_CopilotCacheDir) from being unconditionally overwritten inside targets. - Add new integration-style unit tests that import the real
.targetsfile into a temp project and executedotnet buildto validate key property combinations and error messaging.
Show a summary per file
| File | Description |
|---|---|
| dotnet/src/build/GitHub.Copilot.SDK.targets | Introduces CopilotCliBinaryPath, adjusts target conditions/property assignment, and normalizes copied output filename to copilot[.exe]. |
| dotnet/test/Unit/MSBuildTargetsTests.cs | Adds end-to-end MSBuild-target coverage via subprocess dotnet build against a throwaway imported project. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Also drop RuntimeIdentifier from the subprocess env (the comment said it but only MSBuildSDKsPath was actually removed), so CI workers that set a RID don't pull the build into a different runtimes folder than ExpectedOutputBinary() expects. - Narrow the two best-effort catch clauses (process.Kill on timeout, and Directory.Delete in Dispose) to the specific exception types those operations actually throw, while keeping the same swallow-and- continue semantics. - Sanitize the caller-supplied fileName in WritePreinstalledBinary via Path.GetFileName so a rooted or directory-containing value can't silently escape preinstallDir via Path.Combine. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Cross-SDK Consistency Review ✅This PR modifies only
The changes are purely build infrastructure — adding the No cross-SDK consistency issues. MSBuild targets are inherently .NET-specific; Node.js, Python, and Go users resolve the CLI binary through runtime environment variables (
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #921.
The SDK's MSBuild targets download the Copilot CLI from the npm registry via the
DownloadFiletask, which doesn't support credentials, so consumers behind authenticated npm mirrors fail at build time. The existingCopilotSkipCliDownload=trueescape hatch wasn't a workaround either: it also disabled the copy/register targets, so even if you pre-downloaded the CLI yourself there was no supported way to hand it to the build. On top of that,_CopilotCliBinaryPathwas reassigned unconditionally inside every target, so setting it as a global property had no effect.This change makes the predownloaded-CLI scenario a first-class option without changing default download behavior.
Approach
Add a public
CopilotCliBinaryPathproperty. When set, the targets:_DownloadCopilotCli(no network access);_CopyCopilotCliToOutputand_RegisterCopilotCliForCopyso the supplied binary is placed at the canonicalruntimes/<rid>/native/copilot[.exe]path thatClient.GetBundledCliPathsearches at runtime;<Error>if the path is missing, naming the offending path.The internal
_CopilotCliBinaryPath/_CopilotCacheDirassignments inside each target are now conditioned on emptiness so a caller-supplied value isn't clobbered.The
<Copy>task switches fromDestinationFoldertoDestinationFiles="$(_CopilotOutputDir)\$(_CopilotBinary)"so an off-spec source filename (e.g. one a user extracted from their own mirror) is normalized tocopilot[.exe]on copy. This is safe for the default download path because the npm tarball already extracts to that canonical name.Default behavior with no properties set is unchanged.
Tests
Adds
dotnet/test/Unit/MSBuildTargetsTests.cs, the first MSBuild-target test in the repo. It imports the realGitHub.Copilot.SDK.targetsinto a throwaway csproj under the system temp directory (to avoid inheritingdotnet/Directory.Build.props) and shells out todotnet buildin a subprocess, so MSBuild evaluation, conditions, and the<Copy>task all run for real. Five[Fact]scenarios cover:CopilotCliBinaryPathalone: download skipped, binary copied to the canonical runtimes path with matching contents.copilot[.exe]on copy.CopilotSkipCliDownload=truealone: build succeeds, no binary produced.CopilotCliBinaryPath+CopilotSkipCliDownload=true: still copies to output.CopilotCliBinaryPath: build fails with an error message containing "Copilot CLI binary not found" and the offending path.These are scoped as regression tests for #921 and intentionally don't exercise the network download path; the existing SDK build itself continues to provide implicit smoke coverage of that path.
Validation
dotnet test test\GitHub.Copilot.SDK.Test.csproj-- all 84 unit tests pass, including the 5 new ones.dotnet format --verify-no-changes-- clean.runtimes/win-x64/native/copilot.exe.claude-opus-4.7-xhigh, then a second pass withgpt-5.5andclaude-opus-4.6covering MSBuild correctness, best-practice conformance, and security -- all three reviewers came back with no findings.